Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

335 remove aliases from package index #569

Closed
wants to merge 6 commits into from

Conversation

chlebowa
Copy link
Contributor

@chlebowa chlebowa commented Mar 6, 2024

Fixes #335

Package index is built during the installation process but only once the package has been actually installed.
A file is created, <package>/html/00Index.html, that is the source for the index page.

Here, a function is added that modifies html/00Index.html to remove HTML elements that link to a specified topic through aliases.
The function is called in the loading hook.

@keywords internal is removed from all functions that are included in teal_slice, teal_slices, and filter_state_api.

@chlebowa chlebowa added the core label Mar 6, 2024
@chlebowa chlebowa requested a review from a team March 6, 2024 16:09
Copy link
Contributor

github-actions bot commented Mar 6, 2024

badge

Code Coverage Summary

Filename                        Stmts    Miss  Cover    Missing
----------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------------------------
R/calls_combine_by.R                7       0  100.00%
R/choices_labeled.R                49      14  71.43%   25, 36, 41, 51-56, 68, 72-76
R/count_labels.R                   97       0  100.00%
R/filter_panel_api.R               29       1  96.55%   132
R/FilteredData-utils.R             68      25  63.24%   21-24, 27-30, 52-57, 153, 175-184
R/FilteredData.R                  562     227  59.61%   110, 184, 326, 398, 501-510, 533, 554-595, 613-616, 632, 673-706, 721-723, 727-733, 762-790, 813-815, 819-821, 824-838, 842-852, 855-898, 939, 962-984
R/FilteredDataset-utils.R          23       1  95.65%   125
R/FilteredDataset.R               170      61  64.12%   52, 152, 191, 216-273, 312-314
R/FilteredDatasetDataframe.R      121       8  93.39%   87, 148, 158, 234-238
R/FilteredDatasetDefault.R         18       4  77.78%   103-116
R/FilteredDatasetMAE.R            134      37  72.39%   56, 117-122, 161-166, 170-171, 189-211
R/FilterPanelAPI.R                 10       0  100.00%
R/FilterState-utils.R             101       2  98.02%   264, 294
R/FilterState.R                   361      61  83.10%   89, 212, 230-234, 241-242, 256-257, 263-264, 311, 313, 315, 367, 411, 639, 682-705, 715-734, 769-775, 784-790
R/FilterStateChoices.R            341     108  68.33%   310-313, 325, 367, 389-396, 400-417, 445, 458-469, 481-489, 493-522, 543-546, 549-552, 563-586, 597, 602, 613
R/FilterStateDate.R               212     129  39.15%   227, 279-436
R/FilterStateDatettime.R          309     199  35.60%   266, 318-549
R/FilterStateEmpty.R               53      31  41.51%   89, 99-104, 117, 129-169
R/FilterStateExpr.R                75      62  17.33%   149-272
R/FilterStateLogical.R            196     144  26.53%   136, 158, 218, 222-406
R/FilterStateRange.R              408     105  74.26%   262, 384, 510-514, 517-527, 530, 542-548, 559-571, 575-585, 589-591, 605-632, 647, 650, 664-681, 716-721, 731-733
R/FilterStates-utils.R             70       9  87.14%   108, 127, 188-194, 216, 245
R/FilterStates.R                  364      30  91.76%   78-82, 191, 314-323, 411-414, 457, 542-546, 591, 712-715
R/FilterStatesDF.R                  5       0  100.00%
R/FilterStatesMAE.R                10       1  90.00%   40
R/FilterStatesMatrix.R              3       0  100.00%
R/FilterStatesSE.R                211     157  25.59%   36, 71-73, 83-85, 109-116, 124-131, 154-302
R/include_css_js.R                  5       5  0.00%    12-16
R/remove_aliases.R                 20      20  0.00%    12-39
R/teal_slice.R                    107       4  96.26%   134, 186-187, 197
R/teal_slices.R                    84       5  94.05%   142-147
R/test_utils.R                     21       0  100.00%
R/utils.R                          18       0  100.00%
R/variable_types.R                 15       8  46.67%   44-51
R/zzz.R                            17      17  0.00%    3-48
TOTAL                            4294    1475  65.65%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  --------
R/remove_aliases.R      +20     +20  +100.00%
R/zzz.R                  +1      +1  +100.00%
TOTAL                   +21     +21  -0.32%

Results for commit: bc81ccb

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@pawelru
Copy link
Contributor

pawelru commented Mar 6, 2024

What if 00Index.html file is not writable?
Use case: package is installed in centrally managed directory on the server and users are having read-only access.

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

Then no changes are made

  if (identical(index_file, "") || !isTRUE(utils::file_test("-w", index_file))) {
    return(invisible(FALSE))
  }

@gogonzo
Copy link
Contributor

gogonzo commented Mar 7, 2024

I would like this solution if pkgdown used installed package index - I run pkgdown::build_site and it seems that pkgdown build site from the source including all aliases in the rendered site

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

Maybe for pkgdown we need some custom code in pkgdown.yml to be executed so that the pkgdown-index gets cleaned the same way it's done for package index.

@pawelru
Copy link
Contributor

pawelru commented Mar 7, 2024

Then no changes are made

I am afraid that this would make it working only for subset of users. To my surprise, centrally managed environments is quite popular in the pharma world - even internally we have a BEE environment managed like that.

Is this really the only way to address this problem? Can it be done with modification of roxygen tags? Maybe we can slightly change the approach of documentation to obey this issue? Just thinking loudly...

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

Check this out:

EDIT: my mistake

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

I actually like the idea, if this plays with pkgdown as it also clears aliases for filter-panel-api that are also duplicated (4 times).

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

pkgdown provides a way to exclude a topic from the reference but raises an error if you do 😒

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

Is this really the only way to address this problem? Can it be done with modification of roxygen tags? Maybe we can slightly change the approach of documentation to obey this issue? Just thinking loudly...

Be my guest.

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

@pawelru

Can it be done with modification of roxygen tags?

It's a bigger problem and we researched multiple approaches. You can read #335 and other PR #565

@pawelru
Copy link
Contributor

pawelru commented Mar 7, 2024

Can it be done with modification of roxygen tags?

It's a bigger problem and we researched multiple approaches. You can read #335 and other PR #565

Is this really the only way to address this problem? Can it be done with modification of roxygen tags? Maybe we can slightly change the approach of documentation to obey this issue? Just thinking loudly...

Be my guest.

I tried to avoid going deeper if possible. I need to focus on something else at the current moment. I can do that but a little bit later. Just wanted to flag this out for you in case you are unaware.

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

pkgdown provides a way to exclude a topic from the reference but raises an error if you do 😒

This seems to be a bug that was supposedly fixed in 2022. I will raise the issue again but even if they do fix it, I don't think they will do it soon.

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

I tried to avoid going deeper if possible. I need to focus on something else at the current moment. I can do that but a little bit later. Just wanted to flag this out for you in case you are unaware.

We are four days in, join us when you can 😉

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

pkgdown provides a way to exclude a topic from the reference but raises an error if you do 😒

This seems to be a bug that was supposedly fixed in 2022. I will raise the issue again but even if they do fix it, I don't think they will do it soon

Maybe we can run it with suppressWarnings/suppressMessages to avoid having it be displayed during GHA builds?

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

Maybe we can run it with suppressWarnings/suppressMessages to avoid having it be displayed during GHA builds?

-- Building function reference -------------------------------------------------
Error: 
! in callr subprocess.
Caused by error in `check_missing_topics(rows, pkg)`:
! All topics must be included in reference index
✖ Missing topics: filter_state_api
ℹ Either add to _pkgdown.yml or use @keywords internal
ℹ See `$stdout` for standard output.
Type .Last.error to see the more details.

Good luck.

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

Got it, how do you run it? Did you add reference section to pkgdown.yml and run build_site or did you use build_reference?

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

image

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

Looks like promising path!

@m7pr m7pr mentioned this pull request Mar 7, 2024
@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

@pawelru Re: file permissions, I forgot to mention that a package is test-loaded during installation and the index file is modified then.

Copy link
Contributor

github-actions bot commented Mar 7, 2024

Unit Tests Summary

  1 files   29 suites   23s ⏱️
359 tests 359 ✅ 0 💤 0 ❌
824 runs  824 ✅ 0 💤 0 ❌

Results for commit bc81ccb.

♻️ This comment has been updated with latest results.

@pawelru
Copy link
Contributor

pawelru commented Mar 7, 2024

@pawelru Re: file permissions, I forgot to mention that a package is test-loaded during installation and the index file is modified then.

That's great to hear. It means that my concern is not valid and I no longer see any blockers. However, I do encourage you to continue with looking for alternatives and do compare different approaches. I personally would prefer a fix to be incorporated in the already built package compared to apply fix on install. That's of course as long as it is possible.

Now I cannot say it's buggy or incorrect. It's just non-standard and I am afraid that I am just unable to assess all possible outcomes or edge-cases. Why do I care? Non-standard ways of working introduce some difficulty and requires some/deep knowledge and generally speaking i do want to have a low and cheap maintenance code. I really hope you know what I am mean.
I did a very quick search on GH CRAN mirror: https://github.com/search?q=org%3Acran+path%3A*.R+00Index.html&type=code
Out of 26k+ repos only 16 of them are using 00Index.html file and looking briefly most of them do read only. (It looks like only one knitr does write but I don't want to spend my time trying to understand it - that's not the point to get an exact number).

I know I am sitting on a very comfy reviewer position but I think that there is enough IQ points already there to came up with something good :) You are all clever folks and I believe in you 💪

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

Out of 26k+ repos only 16 of them are using 00Index.html file

False.
The file is built by internal functions of the tools package upon installation. Every package that has a package index page (which I believe means virtually every package in existence) has that page. Why the page is not built with the INDEX file from the package root directory, which can be modified in the source package - I don't know.

I do encourage you to continue with looking for alternatives and do compare different approaches.

Believe me, I tried. This is the best I was able to come up with. See #568 for an inferior alternative.

Now, is this bulletproof? I don't know. For example, my way of dropping rows from the HTML file is rather primitive and makes some assumptions. But here we are.

@pawelru
Copy link
Contributor

pawelru commented Mar 7, 2024

Ahhh I knew you will find something in my statement :) I really tried my best! Let me correct myself. That statement should go as follows

Out of 26k+ packages on CRAN, only 16 are accessing 00Index.html file in its sources. Accessing for read / write or any other purpose. They all might have it (and use it) but only very small number of packages is doing actual modifications.

Anyway, that's just to show why I think it's non-standard. I will try to have a look at this but in the middle of next week the earliest.

@chlebowa
Copy link
Contributor Author

chlebowa commented Mar 7, 2024

Out of 26k+ packages on CRAN, only 16 are accessing 00Index.html file in its sources.

Fair 😃 One of the reasons it is rarely accessed is it does not exist in the source. I agree, it is nonstandard.

@m7pr
Copy link
Contributor

m7pr commented Mar 7, 2024

This is not urgent, let's wait and give @pawelru some time to explore.
@chlebowa I'm amazed how far you went with your research. Totally impressed

@chlebowa
Copy link
Contributor Author

I raised an issue in the pkgdown repo but no help will be coming.

This is what I have been able to accomplish with the current state of affairs:

image

image

I suggest we apply it as is.

@pawelru
Copy link
Contributor

pawelru commented Mar 14, 2024

I just came across this: r-lib/xml2#437

This package cannot be cross compiled, because it tries to evaluate something at install time. Therefore p3m and webr and r-universe need to patch this package, which is a headache

A topic of compilation for WASM binaries. We are not doing right now but I this is something that I really want to (still to be prioritised). Such non-standard fix-on-install type of activities might create a lot of issues down the way.

@chlebowa
Copy link
Contributor Author

I don't think it is relevant.

@chlebowa chlebowa closed this Mar 15, 2024
@insights-engineering-bot insights-engineering-bot deleted the 335_remove_aliases_from_package_index@main branch June 9, 2024 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

teal_slice should show up in package index
4 participants